Skip to content

UI 45579-2: Removes try catch from ILIAS\UI\Implementation\Component\Launcher::getResult method#11208

Open
matheuszych wants to merge 1 commit intoILIAS-eLearning:release_10from
matheuszych:ui/45579-2
Open

UI 45579-2: Removes try catch from ILIAS\UI\Implementation\Component\Launcher::getResult method#11208
matheuszych wants to merge 1 commit intoILIAS-eLearning:release_10from
matheuszych:ui/45579-2

Conversation

@matheuszych
Copy link
Contributor

https://mantis.ilias.de/view.php?id=45579

This is a follow up PR of #10323.

Best regards
@matheuszych

@matheuszych
Copy link
Contributor Author

@thibsy @thojou

@thibsy thibsy added kitchen sink improvement php Pull requests that update Php code labels Feb 27, 2026
@thibsy thibsy self-assigned this Feb 27, 2026
@matheuszych
Copy link
Contributor Author

matheuszych commented Mar 6, 2026

Hello @thibsy,

the reason for why i surrounded return $this→modal?→getForm()?→getInputGroup()?→getContent(); with a try-catch in the first place was, to exactly prevent this type of error (Failed pipeline). The issue is that you can run Inline::getResult without previously attaching the current request to the modal which than leads to a Form handling issue. Previously when Inline::getResult was called, it was checked whether a POST Request was available. If so, it was appended to the modal and the Modals content was returned. If not, null was returned. The resulting behavior thus did not change here. IMHO it should be the users responsibility, to first call Inline::withRequest before calling Inline::getResult. If they do not, they will get null as a result. Inline::withRequest should be the only method responsible for binding the Request to the Modal and Inline Launcher itself (separation of concerns).

If you want to get rid of the try-catch, i would propose something like this. First check if a POST Request is available in the Modal. If not, return null, otherwise, return the content of the Modal. The key difference here is, that the request is not added to the Modal if not specifically intended by the user.

Alternatively, i can revert the Inline::getResult to how it was before.

What do you think?

Best regards
@matheuszych

Copy link
Contributor

@thibsy thibsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @matheuszych,

Thx for the follow-up! I agree, it should be the consumers responsibility to provide a valid request and I also think the separation of concerns was an improvement here.

I do however not like the try-catch block because it simply hides any kind of error which might occur during this process. So from my perspective the current proposal is much better. If there is some error beyond having provided a valid request for this operation, it should definitely tell us that.

There is one minor adjustment I would like:

  • instanceof: please compare the current request against null. While at it, also replace the ternary return with two separate ones using an if block. This IMO improves readability.

Kind regards,
@thibsy (as UI coordinator)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement kitchen sink php Pull requests that update Php code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants